Skip to content

Sol review fixes#61

Merged
constantinpape merged 11 commits into
mainfrom
sol-review-fixes
Jul 12, 2026
Merged

Sol review fixes#61
constantinpape merged 11 commits into
mainfrom
sol-review-fixes

Conversation

@constantinpape

Copy link
Copy Markdown
Contributor

This branch fixes several issues in the distributed graph implementation uncovered by 5.6-sol, verified and fixed by Fable.

constantinpape and others added 11 commits July 11, 2026 15:31
A throw from a worker chunk previously called std::terminate (unhandled
exception in std::thread), and a throw from the thread-0 chunk destroyed
joinable threads — either way aborting the whole process, e.g. for
negative labels in the distributed block scanners with
number_of_threads > 1. Capture the first exception, always join every
worker, and rethrow on the calling thread.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The naive sum_of_squares/count - mean^2 formula suffers catastrophic
cancellation for values with a large baseline and small spread (e.g.
values [1e8, 1e8+1] gave std 0.0 instead of 0.5).

- Distributed: switch the (n, 5) partial-stats layout from
  [count, sum, sum_of_squares, min, max] to the Welford/Chan
  representation [count, mean, M2, min, max]; merge via the Chan
  combine, finalize as variance = M2 / count.
- In-core complex features: compute the variance two-pass from the
  values already stored for the percentiles (exact).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
merge_block_edge_stats copied the entire (E, 5) global accumulator on
every call, making the documented per-block merge loop
O(number_of_blocks x global_edges). Mutate the caller's accumulator in
place instead (and return it, so existing loops keep working): one merge
is now O(block edges) regardless of global graph size. The Python
wrapper requires a C-contiguous writable float64 accumulator and no
longer coerces it (a silent copy would discard the update).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The unordered_set added ~8x node/bucket overhead over the raw edge
data (about 114 MB extra for one million edges). Canonicalize into a
vector and sort + unique instead: same sorted-unique output, peak
memory close to one copy of the input. Document that outputs are valid
inputs, so callers can bound peak memory with a hierarchical merge.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- own_begin / own_shape now require integer dtypes (floats such as
  [0.9, 0] were silently truncated by int()).
- Affinity offsets go through operator.index, which rejects floats.
- merge_edges rejects floating-dtype edge arrays and negative node ids
  in signed arrays (an int64 edge [-1, 2] silently wrapped to
  [2, 2**64 - 1] via the unconditional uint64 cast).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The core statics return the raw _core.UndirectedGraph, so results
lacked the subclass convenience wrappers (find_edges on lists, ...) and
failed isinstance checks against bic.graph.UndirectedGraph. Add a
constructor overload taking (number_of_nodes, uvs, unique) — an
__init__ overload constructs the derived Python class, unlike a static
— and build the subclass classmethods on it. from_edges also gains the
single-pass C++ construction instead of insert_edges.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 2D/3D offset-box sweeps in feature_accumulation.hxx and the
owned-box sweeps in the distributed block extraction duplicated the
same clamp + loop logic. Move a single sweep_clipped_box_{2d,3d} with a
per-axis clip box into detail/grid.hxx; the in-core wrappers pass the
thread slab with unclipped remaining axes, the distributed dispatch
folds the owned box and slab into the clips. Inner loops are unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@constantinpape
constantinpape merged commit 13566a2 into main Jul 12, 2026
6 checks passed
@constantinpape
constantinpape deleted the sol-review-fixes branch July 12, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant